-
Notifications
You must be signed in to change notification settings - Fork 426
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove no-longer-needed transitional cookie code #8865
Conversation
42d0c09
to
82acc01
Compare
Use two separate auth cookies to authenticate requests for HTML pages (these use the pre-existing auth cookie) and requests from h's own frontend code to h's API endpoints (these use a new, separate API auth cookie that's introduced by this commit). This enables the API auth cookie to be more secure. In particular, the API auth cookie can use SameSite=Strict rather than Lax, which gives better protection against CSRF attacks but would not be desirable for the HTML auth cookie (because whenever a user followed a link from another site to Hypothesis they would find themselves not authenticated). How this works: `CookiePolicy` (which is used for HTML page requests) sets and deletes both the HTML auth cookie and the API auth cookie on log in and log out, but reads only the HTML auth cookie to authenticate HTML page requests. `APICookiePolicy` reads only the API auth cookie to authenticate API requests, and never sets or deletes either cookie. In detail: 1. Recall that the top-level security policy (`TopLevelPolicy`) delegates all method calls to either `CookiePolicy` (for HTML endpoints) or `APIPolicy` (for API endpoints). 2. When a user logs in the view calls `pyramid.security.remember()` which calls `TopLevelPolicy.remember()`. The login form submission is an HTML endpoint so `TopLevelPolicy.remember()` delegates to `CookiePolicy.remember()` which sets both the HTML and API auth cookies. 3. Similarly when the user logs out `CookiePolicy.forget()` deletes both the HTML and API auth cookies. 4. To authenticate HTML page requests `CookiePolicy.identity()` reads only the HTML auth cookie. 5. To authenticate API requests `TopLevelPolicy` delegates to `APIPolicy` which delegates to a list of API subpolicies (`BearerTokenPolicy`, `AuthClientPolicy` and `APICookiePolicy`). If there's no bearer token or auth client authentication in the request then `APICookiePolicy` reads only the API auth cookie to authenticate API requests.
8f59437
to
6b5ebeb
Compare
82acc01
to
7d05077
Compare
@seanh - Do you still plan to go ahead and land this as-is following our discussion at the end of last week or do you plan to make any changes? |
We'll need to rebase this if we decide to merge it. I've added a log message (#8889) so we can see whether this transitional cookie code is needed or not. Note that for the next 30 days we'd expect to see this message getting logged, after that maybe not. |
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Remove some code to do with transitioning already-logged-in users to our new separate API auth cookie. This code is no longer needed once all production users have been transitioned.
This PR shouldn't be merged until at least 30 days after #8861 is merged (by then all HTML auth cookies that might have been issued without an API auth cookie being issued by the same login response will have expired, so there won't be any more users logged-in with just an HTML auth cookie and no API one).